Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only check end-entity certificate revocation status on Android #40

Merged
merged 4 commits into from
Nov 17, 2023

Conversation

cpu
Copy link
Member

@cpu cpu commented Nov 2, 2023

Description

This branch proposes an alternative to #34, addressing #13 based on discussion with @complexspaces and others.

In summary, given the constraints1 imposed by the Android platform we feel that checking revocation information for only the end-entity certificate of a given validated certificate chain is the least-worst option. This is a closer match to how Conscrypt handles revocation checking (with the caveat that they also prevent fallback to CRLs while we, at least for now, allow this). In general this approach is easier to understand, and easier to align with other platform verifiers.

This branch updates the Android CertificateVerifier.kt platform specific verifier logic used by the Rust code to configure revocation checking such that we:

  • Only check revocation status for the end entity certificate
  • Prefer OCSP, if stapled or otherwise
  • Allow fallback to CRLs
  • Allow soft failure for network issues

As a result of these changes the logic to determine if a trust anchor was one installed in the system trust store is no longer required, so it falls away along with its dedicated unit test. A new real world verification test case is added for a Let's Encrypt certificate chain that previously failed due to the R3 intermediate in use not providing an AIA OCSP URI. To make keeping the shorted lived EE certificate up-to-date the bash script for fetching valid example certs is generalized to include this test case as well. In the future (#39) it might be nice to rewrite this in Rust.

Footnotes

  1. We can't control revocation checking preferences per-certificate, it must be configured for the whole chain. If we check revocation status for the whole chain preferring OCSP, and a certificate is missing AIA OCSP URI information, it is hard failure. We can't easily check ahead of time whether a given certificate has an AIA OCSP URI. Stapled responses are only available for EE certs. Network access to fetch revocation information can't be controlled (e.g. to only use cached/local information).

cpu added 4 commits November 1, 2023 18:02
This commit add some additional comments to the `CertificateVerifier.kt`
implementation based on the Android developer docs. In particular:

* A note about the return from `checkServerTrusted` being a list ordered
  from EE to trust anchor.

* A note that after adding a `PKIXRevocationChecker` to
  a `PKIXParameters` it can't be modified further or the effects of the
  modifications will be ignored.

* A note about why `isRevocationEnabled` is set to false on the
  `PKIXRevocationChecker`.
This commit updates the `CertificateVerifier.kt` logic used on Android
to change our revocation checking preferences. Now, we only check the
end entity certificate revocation status, preferring OCSP, and allowing
soft failure.

Checking the entire certificate chain's revocation status is infeasible
with the platform options exposed to us: intermediates without complete
authority information access (AIA) information will hard fail.
Performing only EE revocation checking is preferable to more complicated
logic that attempts to decide apriori whether the chain has enough
information to support complete revocation checking or not.

As a result of only checking EE status the logic and unit test for
determining if a certificate is a known root (one installed in the
system trust store) fall away.
In particular this testcase ensures that we can validate a chain from
EE->intermediate->trust anchor for a chain where one or more
certificates (in this case, the intermediate) are missing an authority
information access (AIA) extension that specifies an OCSP access method
and URI.
1. Rename to `update_valid_ee_certs.bash`
2. Don't hardcode location of `bash` for shebang.
3. Remove echo's about potential future extensions.
4. Add a helper function for fetching EE certs.
5. Use helper to update all three valid realworld testcase EE certs
   instead of just `1password_com_valid_1.crt`
Copy link
Collaborator

@complexspaces complexspaces left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I apologize for taking so long to approve this. The code is perfectly fine but I've been getting really bugged about documentation around revocation for the platform verifier, and that threw me off.

In hindsight we should have merged this two weeks ago and followed up with documentation later 😅

@cpu
Copy link
Member Author

cpu commented Nov 17, 2023

@complexspaces Thanks for the review! I understand this is a tricky set of compromises to navigate and taking time to consider carefully them is very reasonable.

In hindsight we should have merged this two weeks ago and followed up with documentation later 😅

Is there any way I can help with this as follow-up? If you have super rough bullet-points about what you'd like covered I could take a crack at drafting some text if it would be helpful. LMK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants